[rocprofiler-compute] Default PC sampling sort to count, add --pc-sampling-rows#7763
Merged
vedithal-amd merged 6 commits intoJun 26, 2026
Conversation
…pling-rows - Default --pc-sampling-sorting-type to count so the analysis table leads with the most-sampled instructions (hotspots). - Add --pc-sampling-rows analyze option (default 10) to cap the PC sampling table; 0 or negative shows all rows. Co-Authored-By: Claude Opus 4 (1M context)
github-actions
Bot
added
documentation
- Validate --pc-sampling-rows as non-negative; 0 shows all rows. - Clarify the 0-means-all behavior in help text, code comment, and docs. Co-Authored-By: Claude Opus 4 (1M context)
- Merge the per-option PC sampling analyze tests into one function. - Assert the negative-rejection message via a mocked stderr instead of capsys. Co-Authored-By: Claude Opus 4 (1M context)
…rror - Assert argparse raises by mocking ArgumentParser.error instead of capturing stderr. Co-Authored-By: Claude Opus 4 (1M context)
- Drop the bespoke fixture and fold the num_rows cases into one parametrized test over the existing helper. Co-Authored-By: Claude Opus 4 (1M context)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates rocprofiler-compute’s PC sampling analysis defaults to better surface hotspots by default, and introduces a new row-limit option to keep CLI output manageable while ensuring consistent behavior across CLI and Web UI.
Changes:
- Defaulted
--pc-sampling-sorting-typefromoffsettocount. - Added
--pc-sampling-rows(default 10) and applied row limiting after sorting in the shared loader. - Updated docs/changelog and expanded unit tests to cover the new defaults and row limiting.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-compute/src/utils/parser.py | Adds num_rows support and applies row limiting after sorting in the shared PC sampling loader. |
| projects/rocprofiler-compute/src/argparser.py | Changes default sorting to count and introduces --pc-sampling-rows with validation. |
| projects/rocprofiler-compute/tests/test_pc_sampling_analysis.py | Adds coverage for num_rows limiting behavior and updates args used by non-metrics table tests. |
| projects/rocprofiler-compute/tests/test_argparser.py | Adds coverage for default/override behavior of the new analyze options and validation of --pc-sampling-rows. |
| projects/rocprofiler-compute/docs/how-to/pc_sampling.rst | Documents new default sorting and the new --pc-sampling-rows option. |
| projects/rocprofiler-compute/CHANGELOG.md | Records the new option and the changed default sorting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cfallows-amd
approved these changes
Jun 24, 2026
cfallows-amd
left a comment
cfallows-amd
left a comment
Contributor
There was a problem hiding this comment.
LGTM, please add two of the suggestions from copilot- update PR description to match implementation for negative error vs all, and the "choices" in argparser to add rigidity when specifying sort type. Marking approved since these are straightforward changes
…ard num_rows Add choices=[offset, count] so invalid sort types fail fast at argparse instead of a later runtime error. Guard the num_rows slice against negative values passed programmatically so the public loader behaves like "show all". Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
vedithal-amd
deleted the
users/vedithal-amd/pc-sampling-rows-default-count
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
End users analyzing PC sampling mostly care about the top hotspots. Defaulting the sort to count surfaces the most-sampled instructions first, and a new --pc-sampling-rows option keeps the table from flooding the CLI by default while letting users widen it as needed.
Technical Details
JIRA ID
AIPROFCOMP-681
Test Plan
Test Result
Unit tests pass locally (199 passed). Ruff clean.
Submission Checklist